Skip to content

Conversation

@michaelmalave
Copy link
Contributor

@michaelmalave michaelmalave commented Oct 16, 2025

Here's the updated PR template with the details of this change:

Summary

Fixed silent failure issue in heroku run:inside and improved error messages in heroku logs when executed outside trusted IP ranges. Added uniform error handling for 403 HTTP responses to provide clear error messages instead of silent failures or generic errors.

Type of Change

  • fix: Bug fix or issue (patch semvar update)
  • feat: Introduces a new feature to the codebase (minor semvar update)
  • perf: Performance improvement
  • docs: Documentation only changes
  • tests: Adding missing tests or correcting existing tests
  • chore: Code cleanup tasks, dependency updates, or other changes

Note: Add a ! after your change type to denote a breaking change.

Testing

yarn && yarn build

Follow guide for staging heroku environment here: staging setup.

get current webserver

heroku ps -a michael-test-fir-app

Confirm ip restrictions (should be empty):

./bin/run spaces:trusted-ips --space michael-test-fir-space
./bin/run spaces:trusted-ips:remove [range] --space michael-test-fir-space

Run commands against test space + app with IP restrictions enabled:

./bin/run run:inside <webserver> -a michael-test-fir-app ls
./bin/run logs -a michael-test-fir-app --tail

Both should return the same error: Error: You can't access this app from your IP address

Additional Context

  • Root Cause:

    • The run:inside command failed silently when IP restrictions were active because the HTTPS request in the SSH connection path received a 403 response but had no handler for it, causing the Promise to hang indefinitely waiting for an upgrade event that never came.
    • The logs command displayed generic error messages for 403 responses because EventSource doesn't expose HTTP response bodies, making it impossible to distinguish between IP restrictions and stream expiration errors.
  • Fix Applied:

    • dyno.ts: Added HTTP response handler in _connect() method to check for 403 status codes and reject the Promise with a user-friendly error message: "You can't access this space from your IP address. Contact your team admin."
    • log-displayer.ts: Implemented fetchHttpResponseBody() utility function that makes a separate HTTP request to fetch the actual response body (since EventSource doesn't expose it). The error handler now:
      • Fetches the 403 response body to extract the server's error message
      • Distinguishes between IP restriction errors and stream expiration errors by checking the response content
      • Uses the server's error message for IP restrictions (e.g., "You can't access this space from your IP address. Contact your team admin.")
      • Falls back to a consistent message for stream expiration: "Your access to the log stream expired. Try again."
      • Also improved the 404 error message to match the stream expiration message for consistency
  • Implementation Details:

    • Used Node's https module directly (matching existing patterns in dyno.ts) to handle staging SSL certificates
    • Added 5-second timeout and proper cleanup to prevent hanging requests
    • Implemented promise state protection to prevent multiple resolve/reject calls

Both commands now provide clear, actionable error messages that match the server's response, maintaining a consistent user experience across commands.

Resolved usage:
image

Related Issue

Closes W-19883467

…ands. Prevents silent failures when commands are executed outside trusted IP ranges
@michaelmalave michaelmalave requested a review from a team as a code owner October 16, 2025 23:03
@michaelmalave michaelmalave changed the title chore: add IP restriction error handling for run:inside and logs commands feat: add IP restriction error handling for run:inside and logs commands Oct 22, 2025
@michaelmalave michaelmalave changed the title feat: add IP restriction error handling for run:inside and logs commands fix: ensure IP restriction error handling for run:inside and logs commands Oct 22, 2025
// For 403 errors, distinguish between stream expiration and IP restrictions
if (isTail) {
// When tailing, 403 typically means stream access expired
msg = 'Log stream access expired. Please try again.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
msg = 'Log stream access expired. Please try again.'
msg = 'Your access to the log stream expired. Try again.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants